feat(core): progress gauge#154
Conversation
📝 WalkthroughWalkthroughAdds a new ChangesGauge feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant HostPage
participant Bundle as projects/core/src/bundle.ts
participant GaugeDefine as projects/core/src/gauge/define.ts
participant Gauge as projects/core/src/gauge/gauge.ts
participant Internals as ElementInternals
HostPage->>Bundle: load core bundle
Bundle->>GaugeDefine: import define.js
GaugeDefine->>Gauge: define(Gauge)
HostPage->>Gauge: set value/max/status/shape/thumb
Gauge->>Gauge: normalize values and compute progress
Gauge->>Internals: set role and aria values
Gauge-->>HostPage: render SVG gauge
Related PRs: None identified. Suggested labels: enhancement, new-component, documentation Suggested reviewers: None identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
projects/core/src/bundle.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. projects/core/src/gauge/define.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. projects/core/src/gauge/gauge.cssESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
5675997 to
e315a0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/vale/styles/config/vocabularies/Elements/accept.txt`:
- Around line 361-362: The Elements vocabulary entry currently includes generic
terms that should not be part of the shared docs vocabulary. Remove indicate and
utilization from the accept list in the Elements vocabulary file, and keep this
vocabulary limited to technical terms, component names, or abbreviations that
are specific to the project.
In `@projects/core/src/progress-gauge/progress-gauge.css`:
- Around line 4-21: The :host rule in progress-gauge.css fails
declaration-empty-line-before because there is no blank line before display;
update the :host block to add the required empty line separating the custom
property declarations from the standard declarations, keeping the fix within the
progress-gauge styles.
In `@projects/core/src/progress-gauge/progress-gauge.examples.ts`:
- Around line 114-123: The Dynamic example script creates an unmanaged interval
and uses a document-wide query, which can stack timers and keep mutating a
detached or wrong gauge on rerender. Update the inline module in
progress-gauge.examples.ts to scope lookups from the example’s own root
container instead of document, and keep a handle to the timer created in the
dynamic gauge example. Add teardown/cleanup for that setInterval when the
example is re-rendered or disposed so only one timer runs per instance.
In `@projects/core/src/progress-gauge/progress-gauge.test.axe.ts`:
- Around line 15-24: The axe test setup only waits for the first
nve-progress-gauge to stabilize, so the remaining gauges may still be rendering
when runAxe executes. Update the beforeEach in progress-gauge.test.axe.ts to
wait for every rendered ProgressGauge instance from the fixture, not just the
element returned by querySelector, so the DOM is fully settled before
accessibility checks run.
In `@projects/core/src/progress-gauge/progress-gauge.test.visual.ts`:
- Around line 19-24: The visual test template is always setting the nve-theme
attribute, even for the default baseline when theme is empty. Update template to
only call document.documentElement.setAttribute in the dark-theme case, and
leave the default document state untouched when theme === ''. Use the template
helper in progress-gauge.test.visual.ts as the place to guard the attribute
assignment.
In `@projects/core/src/progress-gauge/progress-gauge.ts`:
- Around line 22-26: The public version metadata for ProgressGauge still uses a
placeholder release value. Update the `@since` tag in the component docblock and
the ProgressGauge.metadata.version field so they reflect the real published
version instead of 0.0.0, and make the same change anywhere else in this diff
that duplicates the placeholder metadata.
- Around line 49-53: Normalize the progress gauge inputs before they are used in
geometry or ARIA: in progress-gauge.ts, clamp and sanitize `value` and `max` so
`NaN`, negative values, `max <= 0`, and `value > max` cannot flow into
rendering. Update `render()` and `updated()` to reuse the same normalized
numbers when computing the dash geometry and setting
`aria-valuenow`/`aria-valuemax`, using the `value` and `max` properties as the
source symbols to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a3a78298-8f17-4601-b576-e410d040447d
⛔ Files ignored due to path filters (2)
projects/core/.visual/progress-gauge.dark.pngis excluded by!**/*.pngprojects/core/.visual/progress-gauge.pngis excluded by!**/*.png
📒 Files selected for processing (16)
config/vale/styles/config/vocabularies/Elements/accept.txtprojects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/progress-gauge/define.tsprojects/core/src/progress-gauge/index.tsprojects/core/src/progress-gauge/progress-gauge.cssprojects/core/src/progress-gauge/progress-gauge.examples.tsprojects/core/src/progress-gauge/progress-gauge.test.axe.tsprojects/core/src/progress-gauge/progress-gauge.test.lighthouse.tsprojects/core/src/progress-gauge/progress-gauge.test.ssr.tsprojects/core/src/progress-gauge/progress-gauge.test.tsprojects/core/src/progress-gauge/progress-gauge.test.visual.tsprojects/core/src/progress-gauge/progress-gauge.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/progress-gauge.md
| indicate | ||
| utilization |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Keep the Elements vocabulary scoped to docs-specific terms.
indicate and utilization are generic words, so adding them here broadens the shared docs vocabulary without a clear technical need.
As per coding guidelines, add new technical terms, component names, or abbreviations to the Elements vocabulary.
♻️ Suggested change
-indicate
-utilization📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| indicate | |
| utilization |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/vale/styles/config/vocabularies/Elements/accept.txt` around lines 361
- 362, The Elements vocabulary entry currently includes generic terms that
should not be part of the shared docs vocabulary. Remove indicate and
utilization from the accept list in the Elements vocabulary file, and keep this
vocabulary limited to technical terms, component names, or abbreviations that
are specific to the project.
Source: Coding guidelines
e315a0f to
a9cd207
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/progress-gauge/progress-gauge.css`:
- Around line 96-99: The `::slotted(*)` rule in `progress-gauge.css` is
overriding every assigned child’s typography, including consumer-provided styles
like the muted secondary label in the progress gauge example. Remove the blanket
`color` and `font-size` application from the slot styling and let slotted
content inherit defaults instead, keeping any necessary styling scoped to the
gauge’s internal elements rather than all top-level slotted nodes.
In `@projects/core/src/progress-gauge/progress-gauge.test.ts`:
- Around line 18-19: The test setup in progress-gauge.test.ts assigns the result
of ProgressGauge.metadata.tag from querySelector() directly to element, which
can be null under strict typing. Update the setup to guard or assert the
querySelector() result before calling elementIsStable() and before any
_internals access, using the existing element variable and the
ProgressGauge.metadata.tag lookup so a missing custom element fails clearly
instead of crashing later.
In `@projects/core/src/progress-gauge/progress-gauge.ts`:
- Around line 58-59: The `container` property in `ProgressGauge` can receive
arbitrary HTML strings, so `geometry` lookup in `render()` must not assume only
valid values; guard the value before indexing `GAUGE_GEOMETRY` and fall back to
the default geometry unless `container` is exactly `'half'`. Update the
`ProgressGauge` type handling and the `geometry` selection logic together so
`render()` never sees `undefined` for invalid `container` values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ad27c55d-a5d0-47d8-857a-91c33b25067d
⛔ Files ignored due to path filters (2)
projects/core/.visual/progress-gauge.dark.pngis excluded by!**/*.pngprojects/core/.visual/progress-gauge.pngis excluded by!**/*.png
📒 Files selected for processing (15)
projects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/progress-gauge/define.tsprojects/core/src/progress-gauge/index.tsprojects/core/src/progress-gauge/progress-gauge.cssprojects/core/src/progress-gauge/progress-gauge.examples.tsprojects/core/src/progress-gauge/progress-gauge.test.axe.tsprojects/core/src/progress-gauge/progress-gauge.test.lighthouse.tsprojects/core/src/progress-gauge/progress-gauge.test.ssr.tsprojects/core/src/progress-gauge/progress-gauge.test.tsprojects/core/src/progress-gauge/progress-gauge.test.visual.tsprojects/core/src/progress-gauge/progress-gauge.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/progress-gauge.md
| ::slotted(*) { | ||
| color: var(--color); | ||
| font-size: var(--font-size); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not override all consumer slotted typography.
::slotted(*) forces the same color and font-size onto every top-level child. That breaks multi-line content like projects/core/src/progress-gauge/progress-gauge.examples.ts Lines 110-113, where the secondary GPU label is explicitly marked nve-text="body sm muted" but will still render with the primary gauge text styling. Let the slot provide defaults via inheritance instead of restyling every assigned node.
Proposed fix
-::slotted(*) {
- color: var(--color);
- font-size: var(--font-size);
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ::slotted(*) { | |
| color: var(--color); | |
| font-size: var(--font-size); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/progress-gauge/progress-gauge.css` around lines 96 - 99,
The `::slotted(*)` rule in `progress-gauge.css` is overriding every assigned
child’s typography, including consumer-provided styles like the muted secondary
label in the progress gauge example. Remove the blanket `color` and `font-size`
application from the slot styling and let slotted content inherit defaults
instead, keeping any necessary styling scoped to the gauge’s internal elements
rather than all top-level slotted nodes.
| element = fixture.querySelector(ProgressGauge.metadata.tag); | ||
| await elementIsStable(element); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "TypeScript config(s):"
fd -i 'tsconfig*.json' . -x sh -c 'echo "--- $1"; sed -n "1,220p" "$1"' sh {}
echo
echo "Unsafe ProgressGauge fixture lookups:"
rg -n -C2 'querySelector\(ProgressGauge\.metadata\.tag\)' projects/core/src/progress-gauge/progress-gauge.test.tsRepository: NVIDIA/elements
Length of output: 20626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant test file:"
sed -n '1,260p' projects/core/src/progress-gauge/progress-gauge.test.ts
echo
echo "elementIsStable definition:"
rg -n -C3 'function elementIsStable|const elementIsStable|export .*elementIsStable' projects/internals/testing projects/core/src -g '*.ts'
echo
echo "ProgressGauge class and metadata:"
rg -n -C3 'class ProgressGauge|metadata\.tag' projects/core/src/progress-gauge -g '*.ts'Repository: NVIDIA/elements
Length of output: 18735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "TypeScript DOM querySelector typings in installed libs, if available:"
node - <<'JS'
const fs = require('fs');
const path = require('path');
const candidates = [
'node_modules/typescript/lib/lib.dom.d.ts',
'/usr/lib/node_modules/typescript/lib/lib.dom.d.ts',
'/usr/local/lib/node_modules/typescript/lib/lib.dom.d.ts',
];
for (const p of candidates) {
if (fs.existsSync(p)) {
const text = fs.readFileSync(p, 'utf8');
const idx = text.indexOf('querySelector<K extends keyof HTMLElementTagNameMap>');
if (idx !== -1) {
console.log(`FOUND ${p}`);
console.log(text.slice(idx, idx + 700));
process.exit(0);
}
}
}
console.log('lib.dom.d.ts not found in candidate locations');
JSRepository: NVIDIA/elements
Length of output: 986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "createFixture definition:"
sed -n '1,220p' projects/internals/testing/src/index.ts
echo
echo "progress-gauge test fixture patterns in nearby tests:"
rg -n -C2 'fixture\.querySelector\(ProgressGauge\.metadata\.tag\)|querySelector\(ProgressGauge\.metadata\.tag\)' projects/core/src/progress-gauge -g '*.test.ts'Repository: NVIDIA/elements
Length of output: 6212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' projects/core/src/progress-gauge/progress-gauge.tsRepository: NVIDIA/elements
Length of output: 4065
Guard the querySelector() result before assigning element. querySelector() can return null, so these direct assignments are unsafe under strict TS and can turn a missing custom element into an opaque setup crash. Add an assertion/helper before elementIsStable() and the _internals reads.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/progress-gauge/progress-gauge.test.ts` around lines 18 -
19, The test setup in progress-gauge.test.ts assigns the result of
ProgressGauge.metadata.tag from querySelector() directly to element, which can
be null under strict typing. Update the setup to guard or assert the
querySelector() result before calling elementIsStable() and before any
_internals access, using the existing element variable and the
ProgressGauge.metadata.tag lookup so a missing custom element fails clearly
instead of crashing later.
Source: Coding guidelines
| /** Determines the gauge container shape. Set `half` for a compact semi-circular arc. */ | ||
| @property({ type: String, reflect: true }) container?: 'half'; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guard invalid container values before indexing GAUGE_GEOMETRY.
HTML attributes are runtime strings, so <nve-progress-gauge container="foo"> makes geometry undefined and render() throws on Line 83. Fall back explicitly unless the value is exactly 'half'. As per coding guidelines, "When working with TypeScript code, follow /projects/site/src/docs/internal/guidelines/typescript.md for type safety, type guards, discriminated unions, and exhaustive checking."
Suggested fix
- const geometry = GAUGE_GEOMETRY[this.container ?? 'default'];
+ const geometry =
+ this.container === 'half' ? GAUGE_GEOMETRY.half : GAUGE_GEOMETRY.default;Also applies to: 76-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/progress-gauge/progress-gauge.ts` around lines 58 - 59, The
`container` property in `ProgressGauge` can receive arbitrary HTML strings, so
`geometry` lookup in `render()` must not assume only valid values; guard the
value before indexing `GAUGE_GEOMETRY` and fall back to the default geometry
unless `container` is exactly `'half'`. Update the `ProgressGauge` type handling
and the `geometry` selection logic together so `render()` never sees `undefined`
for invalid `container` values.
Source: Coding guidelines
a9cd207 to
5f0872d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/progress-gauge/progress-gauge.test.ts`:
- Around line 119-127: The slot-suppression test in progress-gauge.test.ts is
only checking the slot’s fallback DOM, so it never verifies that projected
content is actually hidden for non-neutral statuses. Update the test case around
the progress-gauge element to render real slotted children before setting
status, then assert on the default slot’s assigned content using the slot
element’s assignedElements() (or the rendered label/visibility state) instead of
childElementCount/textContent. Keep the check tied to the existing
progress-gauge status behavior for warning, success, and danger so it guards the
intended contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9e5ef1a7-3c99-4537-8900-b6b9dd0a8917
⛔ Files ignored due to path filters (2)
projects/core/.visual/progress-gauge.dark.pngis excluded by!**/*.pngprojects/core/.visual/progress-gauge.pngis excluded by!**/*.png
📒 Files selected for processing (15)
projects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/progress-gauge/define.tsprojects/core/src/progress-gauge/index.tsprojects/core/src/progress-gauge/progress-gauge.cssprojects/core/src/progress-gauge/progress-gauge.examples.tsprojects/core/src/progress-gauge/progress-gauge.test.axe.tsprojects/core/src/progress-gauge/progress-gauge.test.lighthouse.tsprojects/core/src/progress-gauge/progress-gauge.test.ssr.tsprojects/core/src/progress-gauge/progress-gauge.test.tsprojects/core/src/progress-gauge/progress-gauge.test.visual.tsprojects/core/src/progress-gauge/progress-gauge.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/progress-gauge.md
| it.each(['warning', 'success', 'danger'] as const)( | ||
| 'should leave the default slot empty for %s status', | ||
| async status => { | ||
| element.status = status; | ||
| await elementIsStable(element); | ||
|
|
||
| const defaultSlot = element.shadowRoot.querySelector('slot:not([name])') as HTMLSlotElement; | ||
| expect(defaultSlot.childElementCount).toBe(0); | ||
| expect(defaultSlot.textContent.trim()).toBe(''); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This slot-suppression test never exercises projected content.
Because the fixture has no slotted children, these assertions pass even if non-neutral statuses still render user content. childElementCount/textContent on the <slot> also only inspect fallback DOM, not assigned nodes. Add slotted content here and assert against assignedElements() (or the rendered label state) so the test actually guards the contract.
Suggested test shape
it.each(['warning', 'success', 'danger'] as const)(
'should leave the default slot empty for %s status',
async status => {
- element.status = status;
+ removeFixture(fixture);
+ fixture = await createFixture(html`
+ <nve-progress-gauge status=${status}>
+ <span>50%</span>
+ </nve-progress-gauge>
+ `);
+ element = fixture.querySelector(ProgressGauge.metadata.tag);
await elementIsStable(element);
const defaultSlot = element.shadowRoot.querySelector('slot:not([name])') as HTMLSlotElement;
- expect(defaultSlot.childElementCount).toBe(0);
- expect(defaultSlot.textContent.trim()).toBe('');
+ expect(defaultSlot.assignedElements()).toHaveLength(0);
}
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it.each(['warning', 'success', 'danger'] as const)( | |
| 'should leave the default slot empty for %s status', | |
| async status => { | |
| element.status = status; | |
| await elementIsStable(element); | |
| const defaultSlot = element.shadowRoot.querySelector('slot:not([name])') as HTMLSlotElement; | |
| expect(defaultSlot.childElementCount).toBe(0); | |
| expect(defaultSlot.textContent.trim()).toBe(''); | |
| it.each(['warning', 'success', 'danger'] as const)( | |
| 'should leave the default slot empty for %s status', | |
| async status => { | |
| removeFixture(fixture); | |
| fixture = await createFixture(html` | |
| <nve-progress-gauge status=${status}> | |
| <span>50%</span> | |
| </nve-progress-gauge> | |
| `); | |
| element = fixture.querySelector(ProgressGauge.metadata.tag); | |
| await elementIsStable(element); | |
| const defaultSlot = element.shadowRoot.querySelector('slot:not([name])') as HTMLSlotElement; | |
| expect(defaultSlot.assignedElements()).toHaveLength(0); | |
| } | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/progress-gauge/progress-gauge.test.ts` around lines 119 -
127, The slot-suppression test in progress-gauge.test.ts is only checking the
slot’s fallback DOM, so it never verifies that projected content is actually
hidden for non-neutral statuses. Update the test case around the progress-gauge
element to render real slotted children before setting status, then assert on
the default slot’s assigned content using the slot element’s assignedElements()
(or the rendered label/visibility state) instead of
childElementCount/textContent. Keep the check tied to the existing
progress-gauge status behavior for warning, success, and danger so it guards the
intended contract.
5f0872d to
40cf763
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/progress-gauge/progress-gauge.ts`:
- Around line 107-111: The ARIA label fallback logic in progress-gauge.ts is
checking the translated value instead of the status, so neutral can incorrectly
use a custom i18n.neutral label. Update the condition around
this._internals.ariaLabel to compare this.status directly against the neutral
status before selecting i18nRecord[this.status], and keep the fallback to
this.i18n.information for neutral or missing labels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a47f12d4-da46-48e9-a631-4d5aa86c590b
⛔ Files ignored due to path filters (2)
projects/core/.visual/progress-gauge.dark.pngis excluded by!**/*.pngprojects/core/.visual/progress-gauge.pngis excluded by!**/*.png
📒 Files selected for processing (15)
projects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/progress-gauge/define.tsprojects/core/src/progress-gauge/index.tsprojects/core/src/progress-gauge/progress-gauge.cssprojects/core/src/progress-gauge/progress-gauge.examples.tsprojects/core/src/progress-gauge/progress-gauge.test.axe.tsprojects/core/src/progress-gauge/progress-gauge.test.lighthouse.tsprojects/core/src/progress-gauge/progress-gauge.test.ssr.tsprojects/core/src/progress-gauge/progress-gauge.test.tsprojects/core/src/progress-gauge/progress-gauge.test.visual.tsprojects/core/src/progress-gauge/progress-gauge.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/progress-gauge.md
| const i18nRecord = this.i18n as Record<string, string | undefined>; | ||
| this._internals.ariaLabel = | ||
| (this.status && i18nRecord[this.status] && i18nRecord[this.status] !== 'neutral' | ||
| ? i18nRecord[this.status]! | ||
| : this.i18n.information) ?? null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ARIA label fallback check compares the resolved i18n string, not the status itself.
i18nRecord[this.status] !== 'neutral' checks whether the translated value differs from the literal string 'neutral', not whether this.status is 'neutral'. This only works today because the default i18n string for neutral happens to equal 'neutral'. Once a consumer customizes i18n.neutral to something else (e.g. via the documented i18n property), the condition becomes true even for neutral status, and the gauge will expose the customized neutral label instead of falling back to this.i18n.information.
🐛 Proposed fix
this._internals.ariaLabel =
- (this.status && i18nRecord[this.status] && i18nRecord[this.status] !== 'neutral'
+ (this.status && this.status !== 'neutral' && i18nRecord[this.status]
? i18nRecord[this.status]!
: this.i18n.information) ?? null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const i18nRecord = this.i18n as Record<string, string | undefined>; | |
| this._internals.ariaLabel = | |
| (this.status && i18nRecord[this.status] && i18nRecord[this.status] !== 'neutral' | |
| ? i18nRecord[this.status]! | |
| : this.i18n.information) ?? null; | |
| const i18nRecord = this.i18n as Record<string, string | undefined>; | |
| this._internals.ariaLabel = | |
| (this.status && this.status !== 'neutral' && i18nRecord[this.status] | |
| ? i18nRecord[this.status]! | |
| : this.i18n.information) ?? null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/progress-gauge/progress-gauge.ts` around lines 107 - 111,
The ARIA label fallback logic in progress-gauge.ts is checking the translated
value instead of the status, so neutral can incorrectly use a custom
i18n.neutral label. Update the condition around this._internals.ariaLabel to
compare this.status directly against the neutral status before selecting
i18nRecord[this.status], and keep the fallback to this.i18n.information for
neutral or missing labels.
Signed-off-by: Cory Rylan <crylan@nvidia.com>
40cf763 to
fb50a2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/gauge/gauge.examples.ts`:
- Around line 122-156: The two gradient definitions inside the render function
for nve-gauge#gauge-temperature-example and nve-gauge#gauge-mbps-example
duplicate the same stop offsets while only changing the color assignments.
Refactor the shared angle pattern into a reusable CSS custom property or shared
style snippet within render, then apply the swapped colors per example so the
gradient structure is defined once and reused consistently.
In `@projects/core/src/gauge/gauge.ts`:
- Around line 151-161: The ARIA label logic in `Gauge.updated` is using a
fragile translated-string sentinel check and an unsafe cast. Update the
condition to check the status value itself (for example, guard `this.status`
against the neutral state) and use proper type narrowing or a type guard instead
of casting `this.i18n` to `Record<string, string | undefined>`. Keep the
`ariaLabel` assignment in `updated` but make it rely on typed access to the
matching i18n entry, with `this.i18n.information` as the fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fc0af875-12a4-4500-860b-7f9083e5fded
⛔ Files ignored due to path filters (2)
projects/core/.visual/gauge.dark.pngis excluded by!**/*.pngprojects/core/.visual/gauge.pngis excluded by!**/*.png
📒 Files selected for processing (16)
config/vale/styles/config/vocabularies/Elements/accept.txtprojects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/gauge/define.tsprojects/core/src/gauge/gauge.cssprojects/core/src/gauge/gauge.examples.tsprojects/core/src/gauge/gauge.test.axe.tsprojects/core/src/gauge/gauge.test.lighthouse.tsprojects/core/src/gauge/gauge.test.ssr.tsprojects/core/src/gauge/gauge.test.tsprojects/core/src/gauge/gauge.test.visual.tsprojects/core/src/gauge/gauge.tsprojects/core/src/gauge/index.tsprojects/core/src/index.test.lighthouse.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/gauge.md
| render: () => html` | ||
| <style> | ||
| nve-gauge#gauge-temperature-example { | ||
| --background: conic-gradient( | ||
| from 225deg at 50% 50%, | ||
| var(--nve-sys-support-success-emphasis-color) 0deg 18deg, | ||
| var(--nve-sys-support-warning-emphasis-color) 135deg, | ||
| var(--nve-sys-support-danger-emphasis-color) 252deg 300deg, | ||
| var(--nve-sys-support-success-emphasis-color) 300deg 360deg | ||
| ); | ||
| } | ||
|
|
||
| nve-gauge#gauge-mbps-example { | ||
| --background: conic-gradient( | ||
| from 225deg at 50% 50%, | ||
| var(--nve-sys-support-danger-emphasis-color) 0deg 18deg, | ||
| var(--nve-sys-support-warning-emphasis-color) 135deg, | ||
| var(--nve-sys-support-success-emphasis-color) 252deg 300deg, | ||
| var(--nve-sys-support-danger-emphasis-color) 300deg 360deg | ||
| ); | ||
| } | ||
| </style> | ||
|
|
||
| <div nve-layout="row gap:md"> | ||
| <nve-gauge thumb="dot" value="82" id="gauge-temperature-example"> | ||
| <span>82°C</span> | ||
| <span nve-text="body sm muted">TEMP</span> | ||
| </nve-gauge> | ||
|
|
||
| <nve-gauge thumb="needle" value="98" id="gauge-mbps-example"> | ||
| <span nve-text="body sm">980Mbps</span> | ||
| <span nve-text="body sm muted">Download</span> | ||
| </nve-gauge> | ||
| </div> | ||
| `}; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Duplicated gradient stop structure between the two <style> blocks.
The #gauge-temperature-example and #gauge-mbps-example gradients (lines 124-132, 134-142) share identical stop offsets and only swap color roles. Consider extracting the shared angle pattern into a CSS custom property (e.g. --gauge-gradient-colors) to reduce duplication if more gradient examples are added later.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/gauge/gauge.examples.ts` around lines 122 - 156, The two
gradient definitions inside the render function for
nve-gauge#gauge-temperature-example and nve-gauge#gauge-mbps-example duplicate
the same stop offsets while only changing the color assignments. Refactor the
shared angle pattern into a reusable CSS custom property or shared style snippet
within render, then apply the swapped colors per example so the gradient
structure is defined once and reused consistently.
| updated(props: PropertyValues<this>) { | ||
| super.updated(props); | ||
| const { value, max } = this.#normalizedValues(); | ||
| this._internals.ariaValueNow = `${value}`; | ||
| this._internals.ariaValueMax = `${max}`; | ||
| const i18nRecord = this.i18n as Record<string, string | undefined>; | ||
| this._internals.ariaLabel = | ||
| (this.status && i18nRecord[this.status] && i18nRecord[this.status] !== 'neutral' | ||
| ? i18nRecord[this.status]! | ||
| : this.i18n.information) ?? null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Fragile sentinel comparison and unsafe cast in ARIA label logic.
The check i18nRecord[this.status] !== 'neutral' compares the translated value against the literal string 'neutral' rather than checking this.status !== 'neutral' directly. If a consumer localizes the neutral key to a string that happens to equal 'neutral' in a different locale, or if the key is renamed, this silently changes behavior. The this.i18n as Record<string, string | undefined> cast also bypasses type safety instead of narrowing this.status with a type guard.
Suggested fix
- const i18nRecord = this.i18n as Record<string, string | undefined>;
- this._internals.ariaLabel =
- (this.status && i18nRecord[this.status] && i18nRecord[this.status] !== 'neutral'
- ? i18nRecord[this.status]!
- : this.i18n.information) ?? null;
+ this._internals.ariaLabel =
+ (this.status && this.status !== 'neutral' ? this.i18n[this.status] : this.i18n.information) ?? null;As per coding guidelines, **/*.ts files should follow typescript.md "for type safety, type guards, discriminated unions, and exhaustive checking."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| updated(props: PropertyValues<this>) { | |
| super.updated(props); | |
| const { value, max } = this.#normalizedValues(); | |
| this._internals.ariaValueNow = `${value}`; | |
| this._internals.ariaValueMax = `${max}`; | |
| const i18nRecord = this.i18n as Record<string, string | undefined>; | |
| this._internals.ariaLabel = | |
| (this.status && i18nRecord[this.status] && i18nRecord[this.status] !== 'neutral' | |
| ? i18nRecord[this.status]! | |
| : this.i18n.information) ?? null; | |
| } | |
| updated(props: PropertyValues<this>) { | |
| super.updated(props); | |
| const { value, max } = this.#normalizedValues(); | |
| this._internals.ariaValueNow = `${value}`; | |
| this._internals.ariaValueMax = `${max}`; | |
| this._internals.ariaLabel = | |
| (this.status && this.status !== 'neutral' ? this.i18n[this.status] : this.i18n.information) ?? null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/gauge/gauge.ts` around lines 151 - 161, The ARIA label
logic in `Gauge.updated` is using a fragile translated-string sentinel check and
an unsafe cast. Update the condition to check the status value itself (for
example, guard `this.status` against the neutral state) and use proper type
narrowing or a type guard instead of casting `this.i18n` to `Record<string,
string | undefined>`. Keep the `ariaLabel` assignment in `updated` but make it
rely on typed access to the matching i18n entry, with `this.i18n.information` as
the fallback.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/vale/styles/config/vocabularies/Elements/accept.txt`:
- Around line 361-362: Add the missing nve-gauge entry to the Elements
vocabulary list in accept.txt so Vale recognizes the new component name used by
gauge.md. Keep it in alphabetical order with the other nve-* entries, placing it
between nve-file and nve-grid, and ensure the list remains consistent with the
component names referenced in the docs.
In `@projects/core/src/gauge/gauge.examples.ts`:
- Around line 186-268: The Dynamic and MultiGauge examples have template literal
markup that is flush-left instead of matching the indentation style used by the
surrounding examples. Reformat the render blocks inside Dynamic.render and
MultiGauge.render so their HTML and script contents are indented consistently
with the other examples in gauge.examples.ts, keeping the structure and behavior
unchanged.
- Around line 186-204: The Dynamic and MultiGauge demo scripts create intervals
that are never cleaned up, so update the embedded script logic in these render
examples to store the interval handle and register cleanup when the demo is
removed. Use the existing Dynamic and MultiGauge render blocks to add a teardown
path that clears the interval on unmount/navigation, and make sure any DOM
references like the queried gauge element are no longer used after cleanup.
In `@projects/core/src/gauge/gauge.test.ts`:
- Around line 102-121: The gauge tests repeat the same fixture
teardown/recreation and stability wait setup in multiple cases, so extract that
flow into a small helper such as renderGauge(template) in gauge.test.ts. Move
the shared removeFixture(fixture), createFixture(...), querySelector, and
elementIsStable steps into the helper, then update the repeated tests to use it
so the Gauge-related test cases stay concise and easier to extend.
- Around line 12-24: The gauge tests assign the result of
fixture.querySelector(Gauge.metadata.tag) directly to element, but that lookup
still returns Gauge | null even though the tag is registered. Update the element
assignments in the Gauge test setup helpers to use a non-null assertion or an
explicit guard before passing the value to elementIsStable, so the code reflects
the guaranteed presence of nve-gauge.
In `@projects/core/src/gauge/gauge.ts`:
- Around line 107-143: The `gauge` template is building inline styles as manual
strings for the `fill-mask`, `fill-dot-end`, and `needle` elements, which should
be switched to Lit’s `styleMap` directive. Update the `gauge.ts` render logic to
pass style objects for the CSS custom properties instead of concatenated
strings, keeping the existing `progressAngle` and `geometry` values but letting
`styleMap` handle serialization. Ensure the relevant bindings in the main render
return are still driven by the same symbols (`progress`, `progressAngle`,
`geometry`, `showDot`, `thumb`) after the change.
In `@projects/core/src/index.test.lighthouse.ts`:
- Line 18: The Lighthouse bundle-size assertion in index.test.lighthouse.ts is
still too strict because index.js measures 132.25 KB, so the threshold needs to
be raised above the observed size. Update the expectation on
report.payload.javascript.requests['index.js'].kb to a value comfortably above
132.25 KB, while keeping the test aligned with the compressed bundle limit being
enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: dc93f0f7-d840-4854-9461-15fe58c1f2fb
⛔ Files ignored due to path filters (2)
projects/core/.visual/gauge.dark.pngis excluded by!**/*.pngprojects/core/.visual/gauge.pngis excluded by!**/*.png
📒 Files selected for processing (16)
config/vale/styles/config/vocabularies/Elements/accept.txtprojects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/gauge/define.tsprojects/core/src/gauge/gauge.cssprojects/core/src/gauge/gauge.examples.tsprojects/core/src/gauge/gauge.test.axe.tsprojects/core/src/gauge/gauge.test.lighthouse.tsprojects/core/src/gauge/gauge.test.ssr.tsprojects/core/src/gauge/gauge.test.tsprojects/core/src/gauge/gauge.test.visual.tsprojects/core/src/gauge/gauge.tsprojects/core/src/gauge/index.tsprojects/core/src/index.test.lighthouse.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/gauge.md
| indicate | ||
| utilization |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing nve-gauge vocabulary entry.
The new gauge.md doc page introduces the nve-gauge tag extensively, but it isn't added to the nve-* component-name list here (alphabetically it belongs between nve-file and nve-grid). This will likely cause Vale to flag the new docs.
📝 Proposed fix
nve-file
+nve-gauge
nve-gridAs per coding guidelines, "When Vale flags new technical terms, component names, or abbreviations as misspelled, add them to config/vale/styles/config/vocabularies/Elements/accept.txt."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/vale/styles/config/vocabularies/Elements/accept.txt` around lines 361
- 362, Add the missing nve-gauge entry to the Elements vocabulary list in
accept.txt so Vale recognizes the new component name used by gauge.md. Keep it
in alphabetical order with the other nve-* entries, placing it between nve-file
and nve-grid, and ensure the list remains consistent with the component names
referenced in the docs.
Source: Coding guidelines
| export const Dynamic = { | ||
| render: () => html` | ||
| <div nve-layout="row gap:sm"> | ||
| <nve-gauge id="dynamic-gpu-gauge" status="success" value="0"> | ||
| <span>0%</span> | ||
| <span nve-text="body sm muted">GPU</span> | ||
| </nve-gauge> | ||
| </div> | ||
| <script type="module"> | ||
| const gauge = document.querySelector('#dynamic-gpu-gauge'); | ||
| const valueElement = gauge.querySelector('span'); | ||
| setInterval(() => { | ||
| const value = Math.floor(Math.random() * 101); | ||
| gauge.value = value; | ||
| gauge.status = value >= 80 ? 'danger' : value >= 60 ? 'warning' : 'success'; | ||
| valueElement.textContent = value + '%'; | ||
| }, 1500); | ||
| </script> | ||
| `}; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Uncleared setInterval in embedded demo scripts.
Both Dynamic and MultiGauge start a setInterval that runs forever with no clearInterval. If the containing element is later removed from the docs page (story switch, navigation), the interval keeps firing against a detached node indefinitely, wasting CPU and retaining references.
♻️ Suggested cleanup pattern
<script type="module">
const gauge = document.querySelector('`#dynamic-gpu-gauge`');
const valueElement = gauge.querySelector('span');
- setInterval(() => {
+ const intervalId = setInterval(() => {
+ if (!gauge.isConnected) {
+ clearInterval(intervalId);
+ return;
+ }
const value = Math.floor(Math.random() * 101);
gauge.value = value;
gauge.status = value >= 80 ? 'danger' : value >= 60 ? 'warning' : 'success';
valueElement.textContent = value + '%';
}, 1500);
</script>Apply the same pattern to the MultiGauge interval.
Also applies to: 210-268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/gauge/gauge.examples.ts` around lines 186 - 204, The
Dynamic and MultiGauge demo scripts create intervals that are never cleaned up,
so update the embedded script logic in these render examples to store the
interval handle and register cleanup when the demo is removed. Use the existing
Dynamic and MultiGauge render blocks to add a teardown path that clears the
interval on unmount/navigation, and make sure any DOM references like the
queried gauge element are no longer used after cleanup.
| export const Dynamic = { | ||
| render: () => html` | ||
| <div nve-layout="row gap:sm"> | ||
| <nve-gauge id="dynamic-gpu-gauge" status="success" value="0"> | ||
| <span>0%</span> | ||
| <span nve-text="body sm muted">GPU</span> | ||
| </nve-gauge> | ||
| </div> | ||
| <script type="module"> | ||
| const gauge = document.querySelector('#dynamic-gpu-gauge'); | ||
| const valueElement = gauge.querySelector('span'); | ||
| setInterval(() => { | ||
| const value = Math.floor(Math.random() * 101); | ||
| gauge.value = value; | ||
| gauge.status = value >= 80 ? 'danger' : value >= 60 ? 'warning' : 'success'; | ||
| valueElement.textContent = value + '%'; | ||
| }, 1500); | ||
| </script> | ||
| `}; | ||
|
|
||
| /** | ||
| * @summary Autonomous vehicle taxi gauges combine battery, perception, compute, and link status with realistic drift. Use for live dispatch views that need changing system health. | ||
| * @tags pattern | ||
| */ | ||
| export const MultiGauge = { | ||
| render: () => html` | ||
| <div nve-layout="row gap:sm"> | ||
| <nve-gauge id="av-taxi-battery-gauge" status="success" value="74"> | ||
| <span>74%</span> | ||
| <span nve-text="body sm muted">BATTERY</span> | ||
| </nve-gauge> | ||
| <nve-gauge id="av-taxi-perception-gauge" status="success" value="96"> | ||
| <span>96%</span> | ||
| <span nve-text="body sm muted">PERCEPTION</span> | ||
| </nve-gauge> | ||
| <nve-gauge id="av-taxi-compute-gauge" status="success" value="62"> | ||
| <span>62%</span> | ||
| <span nve-text="body sm muted">GPU</span> | ||
| </nve-gauge> | ||
| <nve-gauge id="av-taxi-link-gauge" status="success" value="42"> | ||
| <span>42Mbps</span> | ||
| <span nve-text="body sm muted">LINK</span> | ||
| </nve-gauge> | ||
| </div> | ||
| <script type="module"> | ||
| const clamp = (value, min, max) => Math.min(Math.max(value, min), max); | ||
| const telemetry = { | ||
| battery: 74, | ||
| compute: 62, | ||
| link: 42, | ||
| perception: 96 | ||
| }; | ||
| const gauges = { | ||
| battery: document.querySelector('#av-taxi-battery-gauge'), | ||
| compute: document.querySelector('#av-taxi-compute-gauge'), | ||
| link: document.querySelector('#av-taxi-link-gauge'), | ||
| perception: document.querySelector('#av-taxi-perception-gauge') | ||
| }; | ||
|
|
||
| const setGauge = (gauge, value, text, status) => { | ||
| gauge.value = Math.round(value); | ||
| gauge.status = status; | ||
| gauge.querySelector('span').textContent = text; | ||
| }; | ||
|
|
||
| setInterval(() => { | ||
| telemetry.battery = clamp(telemetry.battery - 0.08 + (Math.random() - 0.4) * 0.3, 46, 82); | ||
| telemetry.compute = clamp(telemetry.compute + (Math.random() - 0.48) * 7, 34, 91); | ||
| telemetry.link = clamp(telemetry.link + (Math.random() - 0.5) * 12, 9, 92); | ||
| telemetry.perception = clamp(telemetry.perception + (Math.random() - 0.52) * 2.4, 88, 99); | ||
|
|
||
| const battery = Math.round(telemetry.battery); | ||
| const compute = Math.round(telemetry.compute); | ||
| const link = Math.round(telemetry.link); | ||
| const perception = Math.round(telemetry.perception); | ||
|
|
||
| setGauge(gauges.battery, battery, battery + '%', battery < 30 ? 'danger' : battery < 50 ? 'warning' : 'success'); | ||
| setGauge(gauges.compute, compute, compute + '%', compute >= 85 ? 'danger' : compute >= 70 ? 'warning' : 'success'); | ||
| setGauge(gauges.link, link, link + 'Mbps', link < 15 ? 'danger' : link < 30 ? 'warning' : 'success'); | ||
| setGauge(gauges.perception, perception, perception + '%', perception < 90 ? 'danger' : perception < 94 ? 'warning' : 'success'); | ||
| }, 1600); | ||
| </script> | ||
| `}; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Indentation inconsistent with other examples.
Dynamic and MultiGauge render blocks are flush-left while all preceding examples indent their markup within the template literal. Minor style inconsistency worth aligning for readability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/gauge/gauge.examples.ts` around lines 186 - 268, The
Dynamic and MultiGauge examples have template literal markup that is flush-left
instead of matching the indentation style used by the surrounding examples.
Reformat the render blocks inside Dynamic.render and MultiGauge.render so their
HTML and script contents are indented consistently with the other examples in
gauge.examples.ts, keeping the structure and behavior unchanged.
| let element: Gauge; | ||
|
|
||
| beforeEach(async () => { | ||
| fixture = await createFixture(html` | ||
| <nve-gauge></nve-gauge> | ||
| `); | ||
| element = fixture.querySelector(Gauge.metadata.tag); | ||
| await elementIsStable(element); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| removeFixture(fixture); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check tsconfig strictness and whether HTMLElementTagNameMap is augmented for nve-gauge
fd tsconfig.json --max-depth 3 -x cat {}
fd 'define.ts' projects/core/src/gauge -x cat -n {}Repository: NVIDIA/elements
Length of output: 10683
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gauge.test.ts =="
fd 'gauge.test.ts' projects/core/src -x sed -n '1,380p' {}
echo
echo "== createFixture / elementIsStable definitions =="
rg -n "export (async )?function createFixture|export const createFixture|function elementIsStable|const elementIsStable|export (async )?function removeFixture|export const removeFixture" projects/core/src projects/core/test projects/core -g '!**/dist/**' -g '!**/build/**'
echo
echo "== querySelector typing usages in gauge.test.ts =="
rg -n "querySelector(All)?\(Gauge\.metadata\.tag\)|elementIsStable\(element\)|let element: Gauge" projects/core/src/gauge/gauge.test.ts
echo
echo "== Tag-name map augmentation for nve-gauge =="
rg -n "'nve-gauge'|HTMLElementTagNameMap" projects/core/src -g '!**/dist/**' -g '!**/build/**'Repository: NVIDIA/elements
Length of output: 15752
Use a non-null assertion for the fixture lookup
nve-gauge is already in HTMLElementTagNameMap, but fixture.querySelector(Gauge.metadata.tag) still returns Gauge | null. Assigning it directly to element: Gauge at lines 18, 130, 193, and 329 leaves the null case unchecked. Use ! here or add a guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/gauge/gauge.test.ts` around lines 12 - 24, The gauge tests
assign the result of fixture.querySelector(Gauge.metadata.tag) directly to
element, but that lookup still returns Gauge | null even though the tag is
registered. Update the element assignments in the Gauge test setup helpers to
use a non-null assertion or an explicit guard before passing the value to
elementIsStable, so the code reflects the guaranteed presence of nve-gauge.
Source: Coding guidelines
| it('should scale slotted text with size', async () => { | ||
| removeFixture(fixture); | ||
| fixture = await createFixture(html` | ||
| <div> | ||
| <nve-gauge size="sm"><span data-size="sm">50%</span></nve-gauge> | ||
| <nve-gauge size="md"><span data-size="md">50%</span></nve-gauge> | ||
| <nve-gauge size="lg"><span data-size="lg">50%</span></nve-gauge> | ||
| </div> | ||
| `); | ||
|
|
||
| const gauges = Array.from(fixture.querySelectorAll(Gauge.metadata.tag)) as Gauge[]; | ||
| await Promise.all(gauges.map(gauge => elementIsStable(gauge))); | ||
|
|
||
| const fontSizes = ['sm', 'md', 'lg'].map(size => | ||
| parseFloat(getComputedStyle(fixture.querySelector(`[data-size="${size}"]`) as HTMLElement).fontSize) | ||
| ); | ||
|
|
||
| expect(fontSizes[0]).toBeLessThan(fontSizes[1]); | ||
| expect(fontSizes[1]).toBeLessThan(fontSizes[2]); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate fixture-teardown/recreation boilerplate across tests.
The removeFixture(fixture); fixture = await createFixture(...); element = fixture.querySelector(...); await elementIsStable(element); sequence is repeated four times. Extracting a small renderGauge(template) helper would reduce duplication and ease future maintenance as more variants are added.
♻️ Suggested helper
+async function renderGauge(template: TemplateResult) {
+ const fixture = await createFixture(template);
+ const element = fixture.querySelector(Gauge.metadata.tag) as Gauge;
+ await elementIsStable(element);
+ return { fixture, element };
+}Also applies to: 123-137, 188-194, 322-330
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/gauge/gauge.test.ts` around lines 102 - 121, The gauge
tests repeat the same fixture teardown/recreation and stability wait setup in
multiple cases, so extract that flow into a small helper such as
renderGauge(template) in gauge.test.ts. Move the shared removeFixture(fixture),
createFixture(...), querySelector, and elementIsStable steps into the helper,
then update the repeated tests to use it so the Gauge-related test cases stay
concise and easier to extend.
| return html` | ||
| <div internal-host> | ||
| <svg viewBox=${geometry.viewBox} role="presentation" aria-hidden="true"> | ||
| <defs> | ||
| <mask id="background-mask" maskUnits="userSpaceOnUse" x="0" y="0" width="128" height=${geometry.surfaceHeight}> | ||
| <path pathLength="100" d=${geometry.path} class="background"></path> | ||
| </mask> | ||
| <mask id="fill-mask" maskUnits="userSpaceOnUse" x="0" y="0" width="128" height=${geometry.surfaceHeight}> | ||
| <path pathLength="100" d=${geometry.path} class="gauge" | ||
| ?empty=${progress <= 0} | ||
| style=${`--_progress: ${progress}; --_dash-progress: ${progress * GAUGE_DASH_SCALE}cqw; --_dash-gap: ${GAUGE_DASH_GAP * GAUGE_DASH_SCALE}cqw;`} | ||
| stroke-dasharray=${`${progress} ${GAUGE_DASH_GAP}`}> | ||
| </path> | ||
| </mask> | ||
| </defs> | ||
| <foreignObject width="128" height=${geometry.surfaceHeight} mask="url(#background-mask)"> | ||
| <div xmlns="http://www.w3.org/1999/xhtml" class="background-surface"></div> | ||
| </foreignObject> | ||
| <foreignObject class="fill-layer" width="128" height=${geometry.surfaceHeight} mask="url(#fill-mask)" ?hidden=${!showFill}> | ||
| <div xmlns="http://www.w3.org/1999/xhtml" class="fill-surface"></div> | ||
| </foreignObject> | ||
| <circle class="fill-dot-start" cx=${geometry.start.x} cy=${geometry.start.y} ?hidden=${!(showFill && progress > 0)}></circle> | ||
| <circle class="fill-dot-end" cx=${geometry.center + geometry.radius} cy=${geometry.center} | ||
| ?hidden=${!showDot} | ||
| style=${`--_dot-angle: ${progressAngle}deg; --_dot-origin: ${geometry.center}px ${geometry.center}px; --_dot-start-angle: ${geometry.startAngle}deg;`}> | ||
| </circle> | ||
| <g class="needle" | ||
| ?hidden=${thumb !== 'needle'} | ||
| style=${`--_needle-angle: ${progressAngle}deg; --_needle-origin: ${geometry.center}px ${geometry.center}px; --_needle-start-angle: ${geometry.startAngle}deg;`}> | ||
| <line class="needle-line" x1=${geometry.center} y1=${geometry.center} x2=${geometry.center + 40} y2=${geometry.center}></line> | ||
| <circle class="needle-hub" cx=${geometry.center} cy=${geometry.center}></circle> | ||
| </g> | ||
| </svg> | ||
| <slot></slot> | ||
| </div> | ||
| `; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Prefer styleMap over manual style string interpolation.
The style= bindings on lines 117, 131, and 135 are hand-built template strings. Lit's styleMap directive handles CSS custom properties declaratively and avoids manual string concatenation errors (e.g., missing/extra semicolons).
♻️ Example using styleMap
+import { styleMap } from 'lit/directives/style-map.js';
...
- <path pathLength="100" d=${geometry.path} class="gauge"
- ?empty=${progress <= 0}
- style=${`--_progress: ${progress}; --_dash-progress: ${progress * GAUGE_DASH_SCALE}cqw; --_dash-gap: ${GAUGE_DASH_GAP * GAUGE_DASH_SCALE}cqw;`}
- stroke-dasharray=${`${progress} ${GAUGE_DASH_GAP}`}>
+ <path pathLength="100" d=${geometry.path} class="gauge"
+ ?empty=${progress <= 0}
+ style=${styleMap({
+ '--_progress': `${progress}`,
+ '--_dash-progress': `${progress * GAUGE_DASH_SCALE}cqw`,
+ '--_dash-gap': `${GAUGE_DASH_GAP * GAUGE_DASH_SCALE}cqw`
+ })}
+ stroke-dasharray=${`${progress} ${GAUGE_DASH_GAP}`}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/gauge/gauge.ts` around lines 107 - 143, The `gauge`
template is building inline styles as manual strings for the `fill-mask`,
`fill-dot-end`, and `needle` elements, which should be switched to Lit’s
`styleMap` directive. Update the `gauge.ts` render logic to pass style objects
for the CSS custom properties instead of concatenated strings, keeping the
existing `progressAngle` and `geometry` values but letting `styleMap` handle
serialization. Ensure the relevant bindings in the main render return are still
driven by the same symbols (`progress`, `progressAngle`, `geometry`, `showDot`,
`thumb`) after the change.
| expect(report.scores.accessibility).toBe(100); | ||
| expect(report.scores.bestPractices).toBe(100); | ||
| expect(report.payload.javascript.requests['index.js'].kb).toBeLessThan(130.5); | ||
| expect(report.payload.javascript.requests['index.js'].kb).toBeLessThan(132); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Lighthouse bundle-size threshold still fails CI.
Pipeline logs show index.js measures 132.25 KB, which exceeds the newly raised 132 KB threshold on this line, causing the "JS Bundles should remain within compressed bundle limits" test to fail.
🔧 Proposed fix
- expect(report.payload.javascript.requests['index.js'].kb).toBeLessThan(132);
+ expect(report.payload.javascript.requests['index.js'].kb).toBeLessThan(132.5);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(report.payload.javascript.requests['index.js'].kb).toBeLessThan(132); | |
| expect(report.payload.javascript.requests['index.js'].kb).toBeLessThan(132.5); |
🧰 Tools
🪛 GitHub Check: lighthouse
[failure] 18-18: src/index.test.lighthouse.ts > lighthouse report > JS Bundles should remain within compressed bundle limits
AssertionError: expected 132.25 to be less than 132
❯ src/index.test.lighthouse.ts:18:63
[failure] 18-18: src/index.test.lighthouse.ts > lighthouse report > JS Bundles should remain within compressed bundle limits
AssertionError: expected 132.25 to be less than 132
❯ src/index.test.lighthouse.ts:18:63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/index.test.lighthouse.ts` at line 18, The Lighthouse
bundle-size assertion in index.test.lighthouse.ts is still too strict because
index.js measures 132.25 KB, so the threshold needs to be raised above the
observed size. Update the expectation on
report.payload.javascript.requests['index.js'].kb to a value comfortably above
132.25 KB, while keeping the test aligned with the compressed bundle limit being
enforced.
Source: Pipeline failures
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Tests
Performance